-
-
Notifications
You must be signed in to change notification settings - Fork 3
컴포넌트 접근성 개선(aria-invalid, aria-required) #724
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Bundle ReportChanges will increase total bundle size by 22.63kB (4.08%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: daleui-bundle-esmAssets Changed:
|
c79ed00 to
7dfdb37
Compare
497009a to
27e592b
Compare
@hyoseong1994 동작이 달라도 괜찮다는 말씀이신가요? 추가 검토가 필요하시다는 말씀이신가요? |
| className={css({ display: "flex", flexDirection: "column", gap: "32" })} | ||
| > | ||
| <RadioGroup | ||
| {...args} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
나이스 캐치!
| /** | ||
| * true이면 이 라디오 버튼이 에러 상태가 됩니다. | ||
| * @default false | ||
| */ | ||
| invalid?: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
개별 Radio 컴포넌트에 invalid 속성을 추가했는데, 어떤 사용 케이스를 생각하셨는지 궁금합니다. (그리고 컴포넌트의 API를 변경하는 것은 본 티켓의 목표에서 벗어날 수 있겠다는 생각도 드네요.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
사실 사용케이스를 고민한것은 아니고 figma에 디자인이 되어있는데 해당 기능이 없길래 누락된건가 싶어서 추가했습니다.
개별 라디오에 대해서는 invalid가 필요하지 않겠네요 사용케이스가 없을거같습니다.
API 변경에 대해서 동의합니다. 후속 티켓 생성하여 작업 분리하도록하겠습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hyoseong1994 님, 꼭 디자이너랑 소통하셔서 figma에서도 똑같이 반영되도록 부탁드립다!
| expect(inputElement).not.toHaveAttribute("aria-required"); | ||
| }); | ||
|
|
||
| it("leadingIcon과 trailingIcon이 제공될 때 올바르게 렌더링되어야 합니다.", () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아직도 it 친구가 있었구나!
|
@DaleSeo 안녕하세요 달레님
동작이 달라서 여기만 왜 다른지에 대한 질문이 있을거같아서 작성해둔내용입니다.
|
bd06ec5 to
4aae3ec
Compare
yolophg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수고하셨습니다!
| }); | ||
|
|
||
| test("invalid가 false일 때 오류 메시지가 표시되지 않는다", () => { | ||
| test("invalid가 false일 때 aria-invalid 속성이 올바르게 설정된다", () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
테스트 기준도 더 명확해져서 좋군요👍
개인적인 생각인데, 이 방향을 유지한다면 checkbox처럼 예외가 있는 부분은 간단히 문서로 남겨두면 이후에 맥락 파악하는 데에 도움이 될 것 같아요! |
DaleSeo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안건화 해주신 내용만 결론이 나면 반영해주시면 될 것 같습니다.
변경 사항
목적
리뷰어에게
PR 작성자 체크 리스트